Issue 818: Add legend to map view #935
Conversation
There was a problem hiding this comment.
Thanks @ncallaway, apologies it's taken me a few days to look at this. Overall, really really good. I agree with all the points you made in the Notes section of the PR.
- Bug - the URL can no longer set the legend to open or closed, it's ignored. I believe this is because the code in
modifyStateViaURLQuery()(recomputeReduxState.js) hasn't been updated to reflect the new redux state variable names.
The thing I'm least happy about is how the
<Legend>gets tied to the different redux state
Yes I see what you mean here. Keeping a single redux variable would be cleaner, but necessitate that the map & tree legends always act in sync. Having played with this PR I actually like this behavior and it'd result in much cleaner code.
|
I've merged this in for release with one extra commit: 971ee32: This removes the "for" parameter, resulting in the open/close state of the legends being in-sync. I consider this a feature not a bug. Additionally, the code is simpler & the URL query for "legend" works again. Thanks for the work here Noah, this is really nice 👍 |
|
👍 Sounds great. Sorry I've gone a little dark on nextstrain work for the last couple of weeks. Work-work has been really busy, but I hope to be back picking up some tasks in the next week or so. I'm also interested in helping with NextTrace in any way, so if there's anything y'all need help with there, let me know. |
This adds the legend to the map view as described in #818.
Notes
maplegend or thetreelegend)PR Questions
<Legend>gets tied to the different redux stateLegendis taking aforparameter, which is expected to be one oftreeormapLegenduses that parameter to decide which legend control state variable to use from reduxforparameter, which would reduce the logic coupling, but make the state slightly more complicated.